Refactor Binding data structures in preparation for partition
#54788
Conversation
src/codegen.cpp
Outdated
| jl_value_t *ty = jl_atomic_load_relaxed(&bnd->ty); | ||
| jl_binding_partition_t *bpart = jl_get_binding_partition(bnd, ctx.max_world); | ||
| if (bpart && !bpart->constp) { | ||
| jl_value_t *ty = bpart->restriction; | ||
| if (ty != nullptr) { |
There was a problem hiding this comment.
I believe this needs additional guards now, for this case:
julia> global x::Int64
julia> foo() = (global x; x)
julia> foo() # emits code w/ stores/loads directly to binding->value
julia> const x = 15 # does not update binding->value any more
julia> foo() # should be 15There was a problem hiding this comment.
Invalidation will take care of this.
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
3 times, most recently
from
df97a48
to
dfaa660
Compare
src/julia.h
Outdated
| * First look at ->imported (see above). If NONE, but `->constp`, this holds the | ||
| * value of the constant. Otherwise this is a global and holds the type restriction. | ||
| */ | ||
| jl_value_t *restriction; |
There was a problem hiding this comment.
Would it be worth using Core.Const(...) here, instead of the constp logic?
There was a problem hiding this comment.
I think that would be semantically consistent and reasonable, but there isn't really any benefit to doing so.
| // ->restriction does not hold any import-related data. | ||
| BINDING_IMPORT_NONE = 0x0, | ||
| // Implicit: The binding was implicitly import from a `using`'d module. | ||
| // ->restriction holds the imported binding |
There was a problem hiding this comment.
Is this still a short-circuit link to the defining module, or is it now the immediate module involved in the local using/import?
I'm assuming that we're expecting to maintain backlinks from all the intermediate modules that a binding was resolved through, so that a change to any of them splits all downstream binding partitions?
There was a problem hiding this comment.
Yes, it short circuits. I haven't worked out the backedges yet.
There was a problem hiding this comment.
I changed my mind on this. It no longer short circuits in the latest version.
src/julia.h
Outdated
| uint8_t constp:1; | ||
| uint8_t exportp:1; // `public foo` sets `publicp`, `export foo` sets both `publicp` and `exportp` | ||
| uint8_t publicp:1; // exportp without publicp is not allowed. | ||
| uint8_t imported:1; | ||
| uint8_t imported:2; // enum jl_binding_import | ||
| uint8_t usingfailed:1; |
There was a problem hiding this comment.
Perhaps this should be BINDING_IMPORT_FAILED now
Keno
force-pushed
the
kf/constgloballowerrefactor
branch
3 times, most recently
from
499eed2
to
f00f2e6
Compare
Keno
force-pushed
the
kf/bindingstructrefactor
branch
4 times, most recently
from
890e668
to
c2cc636
Compare
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
Keno
force-pushed
the
kf/bindingstructrefactor
branch
from
aa4b302
to
d1d842e
Compare
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Alright, I've restored all the old semantics, with the exception of and other attempts to redefine constants using setglobal!, which is now an error (and will be after full partition). Constant redefinition using the keyword, i.e. and remains undefined behavior (for now). I'm planning to address the atomics concerns and then merge this in the near future. |
Keno
force-pushed
the
kf/bindingstructrefactor
branch
from
1bfe075
to
e9023c6
Compare
|
I think this should be good to go now. I've gone through and addressed the review comments. I'll run another round of pkgeval. Unless there's something pressing, I think I'll take anything additional post-commit since this is one of those branches that keeps accumulating conflicts and I need to keep the full partition branch up-to-date as well. |
For compat with current semantics.
|
@nanosoldier |
|
@nanosoldier |
This comment was marked as resolved.
This comment was marked as resolved.
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
This seems to break CI consistently: #55642 EDIT: also, the PkgEval above seems to find an "unreachable reached" situation in one or two packages |
This codepath is odd. It derives a method name from the slotname metadata and then does an implicit assignment to that slot. Worse, as the comment indicates, the codepath is missing from the interpreter, which will crash if it were to ever encounter such a piece of code. I suspect this pattern is unused - I accidentally broke it badly in (the as of yet unmerged PR) #54788 and neither tests nor pkgeval noticed. So let's try removing it on master. If it turns out something does depend on it, we can go the opposite way and implement it properly in all the places that look at :method.
This is a re-worked extraction of #54654, adjusted to support the new semantics arrived at over the course of that thread. Note that this is the data-structure change only. The semantics in this PR still match current master to the greatest extent possible. The core idea here is to split `Binding` in two: A new `Binding` with minimal data and a `BindingPartition` that holds all data that is world-age partitioned. In the present PR, these are always in 1:1 correspondednce, but after #54654, there will be multiple `BindingPartition`s for every `Binding`. Essentially the `owner` and `ty` fields have been merged into a new `restriction` field of `BindingPartition`, which may also hold the value of a constant (consistent with the final semantics reached in #54654). The non-partitioned binding->value field is now used exclusively for non-constant globals. The disambiguation for how to interpret the `restriction` field happens via flags. `->imported` grew to 2 bits and can now be one of `NONE`/`IMPLICIT`/ `EXPLICIT`/`GUARD`. `GUARD` corresponds to the old `b->owner == NULL` case. `NONE` corresponds to the old `b->owner == b` case, while IMPLICIT/EXPLICIT correspond to `b->owner != b` and the old `imported` flag. Other than that, the behavior of the flags is unchanged. Additionally, fields are provided for `min_world`/`max_world`/`next`, but these are unused in this PR and prepratory only.
This is a re-worked extraction of #54654, adjusted to support the new semantics arrived at over the course of that thread. Note that this is the data-structure change only. The semantics in this PR still match current master to the greatest extent possible.
The core idea here is to split
Bindingin two: A newBindingwith minimal data and aBindingPartitionthat holds all data that is world-age partitioned. In the present PR, these are always in 1:1 correspondednce, but after #54654, there will be multipleBindingPartitions for everyBinding.Essentially the
ownerandtyfields have been merged into a newrestrictionfield ofBindingPartition, which may also hold the value of a constant (consistent with the final semantics reached in #54654). The non-partitioned binding->value field is now used exclusively for non-constant globals. The disambiguation for how to interpret therestrictionfield happens via flags.->importedgrew to 2 bits and can now be one ofNONE/IMPLICIT/EXPLICIT/GUARD.GUARDcorresponds to the oldb->owner == NULLcase.NONEcorresponds to the oldb->owner == bcase, while IMPLICIT/EXPLICIT correspond tob->owner != band the oldimportedflag. Other than that, the behavior of the flags is unchanged.Additionally, fields are provided for
min_world/max_world/next, but these are unused in this PR and prepratory only.